Include Encryption In-Transit state as part of the desired state hash sent to clients #2837
Conversation
|
/hold for testing |
openshift-ci
bot
added
the
do-not-merge/hold
malayparida2000
changed the title
malayparida2000
force-pushed
the
ms_mode
branch
from
a29a835
to
5c386d0
Compare
|
Should go after #2793 |
malayparida2000
force-pushed
the
ms_mode
branch
3 times, most recently
from
a7e738c
to
d7ddcb0
Compare
malayparida2000
force-pushed
the
ms_mode
branch
from
d7ddcb0
to
1e6fb86
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: malayparida2000 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
malayparida2000
force-pushed
the
ms_mode
branch
from
1e6fb86
to
8805b41
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
| storageCluster := &ocsv1.StorageCluster{} | ||
| storageCluster.Name = "test-storagecluster" | ||
| storageCluster.Namespace = serverNamespace | ||
| assert.NoError(t, client.Create(ctx, storageCluster)) |
There was a problem hiding this comment.
Is this the test for encryption in transit is enabled?
There was a problem hiding this comment.
No this just creates a storagecluster, as I am adding a util function in our code path which expects a storagecluster otherwise it will return an error. This is just to prevent that.
services/provider/server/server.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| desiredClientConfigHash := getDesiredClientConfigHash(channelName, consumerObj, isEncryptionInTransitEnabled(storageCluster.Spec.Network)) |
There was a problem hiding this comment.
This line became too long, please split to multiple lines:
| desiredClientConfigHash := getDesiredClientConfigHash(channelName, consumerObj, isEncryptionInTransitEnabled(storageCluster.Spec.Network)) | |
| desiredClientConfigHash := getDesiredClientConfigHash( | |
| channelName, | |
| consumerObj, | |
| isEncryptionInTransitEnabled(storageCluster.Spec.Network), | |
| ) |
services/provider/server/server.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| desiredClientConfigHash := getDesiredClientConfigHash(channelName, storageConsumer, isEncryptionInTransitEnabled(storageCluster.Spec.Network)) |
There was a problem hiding this comment.
This line became too long, please split to multiple lines:
| desiredClientConfigHash := getDesiredClientConfigHash(channelName, storageConsumer, isEncryptionInTransitEnabled(storageCluster.Spec.Network)) | |
| desiredClientConfigHash := getDesiredClientConfigHash( | |
| channelName, | |
| storageConsumer, | |
| isEncryptionInTransitEnabled(storageCluster.Spec.Network), | |
| ) |
services/provider/server/server.go
Outdated
|
|
||
| return &pb.ReportStatusResponse{ | ||
| DesiredClientOperatorChannel: channelName, | ||
| DesiredConfigHash: desiredClientConfigHash, | ||
| }, nil | ||
| } | ||
|
|
||
| func getDesiredClientConfigHash(channelName string, storageConsumer *ocsv1alpha1.StorageConsumer) string { | ||
| func getDesiredClientConfigHash(channelName string, storageConsumer *ocsv1alpha1.StorageConsumer, inTransitEncryptionEnabled bool) string { |
There was a problem hiding this comment.
- The correct term is
EncryotionInTransiwhich isEiTnotinTransitEncryption - There is no need to mention Enabled for a boolean param, it does not add anything to the readability and just makes the name longer
| func getDesiredClientConfigHash(channelName string, storageConsumer *ocsv1alpha1.StorageConsumer, inTransitEncryptionEnabled bool) string { | |
| func getDesiredClientConfigHash(channelName string, storageConsumer *ocsv1alpha1.StorageConsumer, encryptionInTransit bool) string { |
There was a problem hiding this comment.
Have addressed all the conversations along with this one, PTAL.
services/provider/server/server.go
Outdated
| } | ||
|
|
||
| var foundSc *ocsv1.StorageCluster | ||
|
|
There was a problem hiding this comment.
No need to add empty lines, which all of the lines below belong to the same logical unit
services/provider/server/server.go
Outdated
| } | ||
| foundSc = sc | ||
| } | ||
|
|
There was a problem hiding this comment.
Same as last comment
|
@malayparida2000 Please let the reviewers resolve review comments threads. |
When Encryption in-transit(EiT) is enabled/disabled the kernel mount option for cephFS needs to be updated between prefer-crc/secure. So the desired state hash needs to include the EiT state, so that if the EiT state is changed the desired state hash will change and the client will reconcile to get the updated mount option. Signed-off-by: Malay Kumar Parida <[email protected]>
malayparida2000
force-pushed
the
ms_mode
branch
from
8805b41
to
27ad9f0
Compare
|
/retest |
When in-transit encryption is enabled/disabled the kernel mount option for cephFS needs to be updated between prefer-crc/secure. So the desired state hash needs to include the EiT state, so that if the EiT state is changed the desired state hash will change and the client will reconcile to get the updated mount option.